Reject non-finite input in hash_rate_to_target - #2221
Conversation
hash_rate_to_target guards only against zero/negative share_per_min and negative hashrate. It does NOT screen for non-finite values, and the intermediate `h * s as u128` cast saturates silently: `NaN as u128` is 0 and `f64::INFINITY as u128` is u128::MAX. So a NaN or infinite argument slips past every existing guard and yields a garbage target with no error. The +inf case is the dangerous one. A +inf hashrate casts to the maximum possible work, which collapses the target toward zero — the HARDEST difficulty the function can emit. So a single non-finite hashrate silently produces a maximally over-difficult target (the over-difficulty direction), from a converter whose contract is to reject bad input. NaN and 0.0 produce the easiest target instead, but the asymmetry is the point: one non-finite input drives difficulty to the ceiling. Reject non-finite hashrate AND share_per_min up front, with a dedicated HashRateToTargetError::NonFiniteInput variant. The check is ordered FIRST, ahead of the zero/negative checks: a NaN compares false to 0.0 and is sign-positive, and -inf is sign-negative, so without the leading finite screen they would fall through to DivisionByZero / NegativeInput / the cast. The ordering is the soundness property and is pinned by a test. Pure robustness fix — input validation only, no behavioral change to any caller's control path. 6 unit tests: each non-finite value on each operand, the load-bearing ordering (-inf and NaN screened as NonFiniteInput, not Negative/DivisionByZero), the +inf-yields-no-target headline, and that the pre-existing finite guards and valid conversions are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ning#2221 class) Records the bounded result of a parallel source-audit of channels_sv2's public input surface for the stratum-mining#2221 bug class (functions accepting non-finite/out-of-range numeric input that produce garbage/dangerous control values). Confirmed independent findings: A (vardiff new_with_min NaN-disables-clamp) + B (try_vardiff divisor finiteness), both fixed on branch fix/vardiff-reject-non-finite-hashrate; C (server set_nominal_hashrate unvalidated store) as a defense-in-depth note. Six garbage-target caller sites are the blast radius of stratum-mining#2221 (closed on its merge), not new findings. Cleared the false suspects (hash_rate_from_target, client setters, share-accounting sums) — the cleared set outnumbers the confirmed. Sequencing decided: let stratum-mining#2221 merge first, prepare A+B ready-but-unopened, then let maintainers' convention choose the tracking construct. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
par1ram
left a comment
There was a problem hiding this comment.
I found one remaining correctness gap in the new validation. The guard at target.rs:93 validates only the operands, not the derived work value. Both inputs can be finite while 60.0 / share_per_min or the subsequent multiplication is non-finite or larger than u128::MAX. On the current merge with main, hash_rate_to_target(f64::from(f32::MAX), 1.0)—within the domain of the existing f32 channel callers—passes the guard, saturates to u128::MAX, and panics at target.rs:119 on h_times_s + 1 in debug builds; release builds continue with a garbage target. Please validate the computed floating-point work for finiteness and representability, with room for the + 1, before casting, return an error, and add this case as a regression test. I reproduced this with a catch_unwind regression test. The existing 83 crate tests, cargo clippy -p channels_sv2 --lib -- -D warnings, and the no_std check otherwise pass.
Reject non-finite input in
hash_rate_to_targetSummary
channels_sv2::target::hash_rate_to_targetaccepts non-finite arguments(
NaN,±infinity) and silently produces a garbage target instead ofreturning an error. For a
+infhashrate the garbage target is the hardestthe function can emit — it drives difficulty to the ceiling. This PR rejects
non-finite input up front with a dedicated error variant. It is a pure
input-validation fix: no behavioral change to any caller's control path.
The bug, and why
+infis the dangerous oneThe function guards only three cases today:
None of these screen for finiteness, and the intermediate cast saturates
silently:
So a non-finite argument slips past every guard and reaches the target
arithmetic. The three non-finite hashrate inputs resolve as:
as u1280.0(already accepted)0NaN0+infu128::MAX0The easy cases (
NaN,0.0) are self-correcting — an over-easy target makesthe miner over-produce shares and the controller tightens back up. The
+infcase is not. It silently emits the maximally over-difficult target — the
over-difficulty direction — from a converter whose contract is to reject bad
input. A miner whose telemetry reports
+infhashrate (a divide-by-zero in ahashrate estimate, an overflow, an uninitialized sensor) would be handed the
tightest possible difficulty with no error raised anywhere. That is the failure
worth closing at the source: a single non-finite input driving difficulty to the
ceiling.
The fix
Reject non-finite
hashrateandshare_per_minbefore any arithmetic, witha dedicated variant:
Two details that matter:
share_per_minreaches the samesaturating cast path (
60.0 / share_per_minthen into the work term), soscreening only the hashrate would leave the symmetric hole open.
NaNcompares false to
0.0and is sign-positive, so it would fall through theexisting
== 0.0andis_sign_negative()checks to the cast; a-infissign-negative and would be mis-reported as
NegativeInput. Putting the finitescreen ahead of the others is what makes the rejection correct and the error
variant accurate. A test pins this ordering.
A new
HashRateToTargetError::NonFiniteInputvariant (rather than overloadingNegativeInput) keeps the fault honest for callers: "you passedNaN" is adifferent error than "you passed a negative number."
Tests
6 unit tests in
target.rs(the function had no test module previously):non_finite_hashrate_is_rejected—NaN/+inf/-infhashrate →NonFiniteInputnon_finite_share_per_min_is_rejected— same forshare_per_min(both operands)neg_infinity_is_non_finite_not_negative— the load-bearing ordering:-infand a
NaNshare_per_minare reportedNonFiniteInput, notNegativeInput/
DivisionByZeropositive_infinity_hashrate_does_not_yield_a_target— the headline case: a+infhashrate no longer produces the hardest-difficulty targetfinite_input_still_converts/preexisting_finite_guards_unchanged—regression guards: ordinary finite conversion still works, zero hashrate is
still accepted, and the pre-existing
NegativeInput/DivisionByZeroguardsare unchanged
Scope
sv2/channels-sv2/src/target.rs(+~20 lines of fix, the resttests). No other crate consumes
HashRateToTargetErrorby exhaustive match,so the added variant is non-breaking.
how any controller responds to a valid hint; it rejects inputs that were
never valid. (Hint-handling policy — how a consumer should react to a
difficulty revision — is a separate concern and deliberately out of scope here.)